[opt](paimon) Reduce JNI read object allocations - #66244
Merged
Gabriel39 merged 2 commits intoJul 30, 2026
Merged
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Gabriel39
marked this pull request as ready for review
July 29, 2026 14:10
Contributor
TPC-H: Total hot run time: 29357 ms |
Contributor
TPC-DS: Total hot run time: 177045 ms |
Contributor
ClickBench: Total hot run time: 25 s |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Request changes.
Two blocking issues remain:
- The eagerly cached
ZoneIdrejects Doris-supported short session-zone aliases before any value is read, including scalar-only Paimon scans. - Recursive per-position caches retain the historical union of nested shapes for the scanner lifetime, so valid shifting nested data can require multiplicatively more heap than any individual row.
Critical checkpoint conclusions:
- Goal and proof: Reusing wrappers and caching timestamp conversion state reduces allocations for stable shapes, and the new unit assertions cover identity reuse and selected temporal boundaries. The two findings above show the optimization is not yet safe over the full production input/session domain.
- Scope and clarity: The PR is focused on two Paimon scanner files and uses one shared reuse helper. No unrelated changes were found.
- Concurrency: Each Java scanner is driven serially by its owning JNI reader, and
VectorColumnrecursively consumes unpacked values synchronously before rebinding. No race, lock, or deadlock issue was found. - Lifecycle and memory: Active wrappers are rebound correctly, but inactive recursive caches survive shrink/null transitions and
releaseBatch, producing the reported scanner-lifetime historical-shape retention. - Compatibility and error behavior: No persisted, storage-format, RPC, or JNI layout change is involved, so no rolling-upgrade shim is needed. The eager one-argument zone parse is a behavior compatibility regression for valid Doris sessions; JNI error propagation otherwise remains intact.
- Parallel and conditional paths: Arrays, maps, structs, sparse struct indexes, nesting, nulls, grow/shrink emission, mixed top-level schemas, both Paimon timestamp mappings, and both JNI reader generations were traced. The timestamp fast paths are algebraically equivalent to the previous UTC conversions; no separate timestamp-value bug was found.
- Tests and results: The added expected values are correct, but coverage misses a Doris short-zone scanner construction and shifting nested high-water marks/retained heap. No builds or tests were run in this review-only environment, as required by the review instructions.
- Performance and observability: Stable-shape allocation reductions are credible, but the benchmark's fixed 16-element shape does not bound historical recursive retention. Existing scanner/JVM metrics add no distinct blocker beyond fixing that retention.
- Configuration, transactions, persistence, data writes, FE-BE variable additions, static initialization, and master failover are not applicable.
User focus: no additional focus was provided; the complete PR was reviewed.
Contributor
FE Regression Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: N/A Related PR: apache#66244 Problem Summary: Eager ZoneId parsing rejected Doris-supported short IDs such as EST and interpreted CST differently from FE. Recursive position caches also retained the historical union of nested collection shapes, allowing wrappers and old container references to grow as a large child moved between outer positions. Resolve zones with Doris-equivalent aliases, trim array and map caches to the current container size, and release descendants when a complex value becomes null. ### Release note None ### Check List (For Author) - Test: Unit Test - Behavior changed: No; restores existing timezone compatibility and bounds internal cache retention. - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Request changes.
One blocking performance issue remains: scanner-wide cache ownership lets later projected fields trim or clear wrappers warmed by earlier fields, so stable multi-column ARRAY/MAP scans can continue allocating O(rows * elements). See the inline comment.
Critical checkpoint conclusions:
- Goal and proof: Per-position wrapper reuse and the cached ZoneId reduce allocations for a single stable complex field, and the changed expectations correctly cover local reuse, nested pruning, timezone aliases, negative epochs, nanoseconds, and DST. The production multi-field lifecycle is not covered and fails the stated allocation/GC goal in the reported path.
- Scope and clarity: The change is localized to the Paimon value adapter and its tests. The remaining problem is that cache state is attached to the scanner-wide mutable root rather than to the projected field whose shape it represents.
- Concurrency and lifecycle: Both JNI reader paths drive each scanner serially, and VectorColumn recursively consumes every unpacked wrapper before the root is rebound. No race, deadlock, stale-value read, close leak, or asynchronous retention issue was found. The prior historical nested-shape retention concern is fixed by trimming and null-descendant clearing within one value.
- Compatibility and error behavior: No wire, storage, persistence, symbol, configuration, transaction, or rolling-upgrade contract changes are involved. The local timezone aliases match FE's current accepted short IDs, including Doris CST semantics, and the timestamp simplifications are equivalent to the previous conversions. Invalid zones still fail explicitly.
- Parallel and special paths: ARRAY, MAP, STRUCT, nested values, sparse struct projections, nulls, grow/shrink/empty transitions, both timestamp mappings, and both native JNI reader generations were traced. ARRAY and MAP share the inline cross-field eviction defect; no separate STRUCT correctness issue was substantiated.
- Tests and results: The new direct-wrapper test results are internally correct, and the author reports 22 Paimon tests plus checkstyle passing. I did not rerun builds or tests because this runner is review-only. Please add mixed-column lifecycle coverage for differently sized ARRAY/MAP roots and for a stable complex field followed by a null scalar.
- Performance and observability: The allocation reductions are credible when a cache survives, but the accepted issue can restore per-row wrapper allocation on ordinary stable schemas and can add cache-list churn versus the baseline. No additional logging or metric blocker was found.
- Data writes, EditLog, master failover, visible-version/MoW behavior, FE-BE variable addition, dynamic configuration, and static initialization ordering are not applicable.
User focus: no additional focus was provided; the complete PR was reviewed.
| private ZoneId timeZone; | ||
| // Keep these caches lazy so scalar columns do not pay for complex-type reuse bookkeeping. | ||
| private List<PaimonColumnValue> arrayValues; | ||
| private List<PaimonColumnValue> mapKeys; |
Contributor
There was a problem hiding this comment.
These caches belong to the scanner-wide root value, which PaimonJniScanner reassigns to every projected field. For stable a ARRAY<INT> of size N followed by empty b ARRAY<INT>, b trims this same arrayValues to zero, so the next row recreates all N wrappers for a; MAP has the same issue. A later null scalar is worse: isNull() clears all four cache families. Thus ordinary multi-column scans can still allocate O(rows * elements) wrappers despite stable shapes. Please scope the root cache graph per projected column (while keeping recursive pruning within each graph) and add mixed-column tests covering different collection sizes and a later null scalar.
zhangstar333
approved these changes
Jul 30, 2026
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
Contributor
TPC-H: Total hot run time: 29527 ms |
Contributor
FE UT Coverage ReportIncrement line coverage `` 🎉 |
Contributor
TPC-DS: Total hot run time: 177995 ms |
Contributor
ClickBench: Total hot run time: 24.89 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: N/A
Related PR: #66223
Problem Summary:
The Paimon JNI read path created a new
PaimonColumnValuefor every array element, map key/value, and struct field on every row. Timestamp conversions also resolved the session time zone per value and built redundant intermediate temporal objects. On wide nested data or large scan batches, these short-lived objects increase allocation rate and GC pressure.For example, consider two consecutive
ARRAY<INT>rows,[10, 20]and[30, 40]. Previously, reading the second row allocated two new wrappers even though the first row's wrappers had already been consumed. This PR keeps two position-specific wrappers and resets their record, index, Doris type, Paimon type, and time zone before reading the second row. This is safe becauseVectorColumnconsumes the unpacked list synchronously before the scanner advances to another value.The same invariant is applied separately to array elements, map keys, map values, and struct fields. Caches are lazy, grow only when needed, and only the current collection size or projected struct indexes are emitted, so shrinking collections cannot expose stale entries. Struct wrappers are cached by the original field index, preserving sparse/non-contiguous projection behavior fixed by #66223.
For temporal values, the session time-zone string is now parsed once into a
ZoneId. A PaimonTIMESTAMP_LTZepoch instant is converted directly into that zone instead of constructing UTC and target-zone intermediates.TIMESTAMPTZuses Paimon's equivalent local representation directly instead of converting throughInstantand UTC.What changed?
PaimonColumnValuewrappers across rows for arrays, maps, structs, and recursively nested values.ZoneIdinstead of resolving it for every value.Local allocation and latency microbenchmark
Environment: Linux x86_64, Oracle JDK 17.0.16, Paimon 1.3.1. The benchmark ran latest
masterand this commit in separate worktrees on the same machine. Each case used 100,000 warmup iterations and 5 measured trials; the table reports medians. Collection cases ran 300,000 operations per trial, timestamp cases ran 1,000,000. Allocation was measured withThreadMXBean; GC count/time was measured withGarbageCollectorMXBean. A full GC was requested before each measured trial and excluded from the reported GC delta.The collection output lists were preallocated and cleared between operations to isolate allocation inside the Paimon value conversion path. Therefore these numbers are a focused Java microbenchmark, not an end-to-end query throughput claim.
Benchmark data shape and values:
ARRAY<INT>[16]: oneGenericRowcontaining oneGenericArraywith[0, 1, ..., 15].MAP<INT,BIGINT>[16]: oneGenericRowcontaining one insertion-orderedGenericMapwith{0: 0, 1: 1, ..., 15: 15}.STRUCT<INT,STRING,BIGINT>: one binary row containing(10, "x", 100); all three fields are projected.TIMESTAMP_LTZ(9): one fixed instant,2024-03-10T10:30:00.123456789Z, read in session zoneAmerica/Los_Angeles. This date exercises the US DST transition day.TIMESTAMPTZ(9): one fixed pre-epoch value,1969-12-31T23:59:59.999999999Z, read in UTC.Every benchmark operation rereads the same Paimon row/value. This intentionally removes file I/O, row construction, and table scan variability, and isolates the allocation/latency of
PaimonColumnValue.unpack*()or timestamp conversion. The core collection loop is equivalent to:On
master, eachunpackArraycall above creates 16 wrapper objects. With this PR, the warmup creates the 16 position-specific wrappers once and measured iterations only reset them. Map and struct use the same loop shape with separate key/value or projected-field output lists.ARRAY<INT>[16]MAP<INT,BIGINT>[16]STRUCT<INT,STRING,BIGINT>TIMESTAMP_LTZ(9)TIMESTAMPTZ(9)The remaining map allocation comes primarily from Paimon's
GenericMap.keyArray()/valueArray()materialization, which is unchanged by this PR. Struct latency is effectively unchanged while allocation drops by 75%.Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Validation command:
Result: 22 tests, 0 failures, 0 errors, 0 skipped.
Check List (For Reviewer who merge this PR)
\n\n### Review follow-up\n\nTwo review findings are addressed in
4459f2e9749:\n\n- Session zones are resolved with the same short-ID aliases accepted by Doris FE. For example, an INT-only scan withESTno longer fails during scanner construction, andCSTkeeps Doris semantics (Asia/Shanghai) instead of Java\u0027s US Central interpretation.\n- Array/map caches are trimmed when a collection shrinks, and a null complex value releases all descendant caches. This keeps recursive retention proportional to the live shape while preserving position reuse for stable shapes.\n\nRetention test data shape:ARRAY<ARRAY<INT>>with outer sizeN=4and one inner array of sizeM=32; the large child moves from position 0 to 1 to 2, with the abandoned positions exercising both an empty array and null. Before the fix, retained wrappers grew from 36 to 68 after the first move and could grow towardN + N*M; after the fix they remain 36 (N + M) at every step. This test measures retained wrapper graph size rather than heap bytes, so it is deterministic and directly checks the leak shape that the fixed-size throughput benchmark missed.\n\nValidation: all threepaimon-scannertest classes passed (22 tests, 0 failures/errors), including short-zone construction, CST conversion, shrink/null pruning, existing wrapper identity reuse, arrays, maps, structs, and timestamp boundaries. Checkstyle reported 0 violations. An unfiltered reactor attempt was blocked before reachingpaimon-scannerby the existing nativejava-commonJniScannerTestprocess exiting with code 134; the module-scopedPaimon*Testrun completed successfully.\n